I wrote a test for the nut13 functionality and ran it on the commit where it was broken...and it passed! Turns out we were reducing the keyset ID to 32 bits twice, once in the TryFrom impl and again in derive_path_from_keyset_id. Now with a unit test we can be sure that deriving the path works correctly and only reduce the keyset to u32 once.

PS I'm having to use --no-verify to commit. Not sure how to fix it but I can run all the checks using just.

Not sure I agree with you about leaving out the u64 TryFrom impl but you are the captain. Disagree and commit. 🫡
https://en.wikipedia.org/wiki/Disagree_and_commit

Turns out we were reducing the keyset ID to 32 bits twice, once in the TryFrom impl and again in derive_path_from_keyset_id

Yeah I noticed that, I'm gonna write some full integration tests deriving from seed to make sure we haven't changed that before we merge this.

Already a test for this.

Not sure I agree with you about leaving out the u64 TryFrom impl but you are the captain. Disagree and commit. 🫡

Happy to hear a counter argument.

PS I'm having to use --no-verify to commit. Not sure how to fix it but I can run all the checks using just.

I know precommit still giving issues going to fix that here #451

Noticed while we're here we can actually make it a From instead of a TryFrom if we add that

    pub fn as_bytes(&self) -> [u8; Self::BYTELEN + 1] {
        let mut bytes = [0u8; Self::BYTELEN + 1];
        bytes[0] = self.version.to_byte();
        bytes[1..].copy_from_slice(&self.id);
        bytes
    }

to

Can do this as a different pr if you don't want to include it here.

Nice!

Happy to hear a counter argument.

If we strictly limit cdk to what is in the specs then we leave out nice-to-have features like this because this is an implementation detail that doesn't belong in the spec.

I think converting 8 bytes to a u64 is a useful little helper function. You are correct that it's not a big deal to implement outside of cdk but this decision pushes more complexity up the stack. I think the point of this project is to push as much implementation complexity as we can down stack to free up higher level devs to focus on other problems, enabling them to build more cool shit. To me, it feels like it belongs in a 'dev kit' library.

OTOH I'm new to cdk, new to rust, and I've never contributed to a low level library, so what do I know? I just have my engineering intuition to rely on. It's a really minor thing at this point, not worth a drawn out discussion.

Another potential reason not to implement From<Id> for u64 and it's inverse is that it could create confusion with From<Id> for u32. This is best solved with code documentation, IMO.

Another potential reason not to implement From for u64 and it's inverse is that it could create confusion with From for u32. This is best solved with code documentation, IMO.

This is my issue with it - not that it's not in the spec but that it's potentially in conflict with what is. If someone has a u32 they may expect they can safely convert it to u64 to do From for Id, but that conversion path isn't actually safe since they can't know if data was lost when converting from Id to u32 via From for u32. Having these functions is too likely to lead to potential bugs, and adding docs isn't a reliable safeguard as documentation can be easily overlooked.

While I understand the desire to reduce complexity for higher-level developers and that is the goal of cdk, in this case the risk of subtle bugs outweighs the benefit. Even though implementing this trivial conversion outside cdk requires more work, it's preferable to having potentially unsafe implicit conversions in the library itself. This is especially important for a low-level library where predictability and safety should take precedence over convenience.

Gotcha, makes sense.

If someone has a u32 they may expect they can safely convert it to u64 to do From for Id

Yeah I thought that was where you were gonna go initially. What do you think about this?

impl From<u32> for Id {
    fn from(value: Id) -> Self {
        panic!("Conversion from u32 to Id is not allowed. This operation is invalid and should never be performed.");
    }
}

In general, we don't use panic in cdk other then tests always return an error.

Here specifically, I don't think there is any need to add a function that will always panic or return an error the omission of a function supporting it implicitly signals it can't/shouldn't be done. This also introduces a runtime panic instead of a developer trying to do the conversion and realizing there is no function for it when developing a much preferable time for this realization.

If we want to be more explicit about it being a one way function and why we can add to that documentation on the From<Id> for u32